Skip to content

feat(file-input): Added file input component #1613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
Apr 3, 2025

Conversation

simeonoff
Copy link
Collaborator

Closes #1581

@simeonoff simeonoff changed the title feat(input): add file type to the allowed types feat(file-input): add new component Mar 26, 2025
@simeonoff simeonoff marked this pull request as ready for review March 31, 2025 14:06
Copy link
Member

@rkaraivanov rkaraivanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with some minor style suggestions.

The main concern is a UX issue with required file inputs. When a user clicks a required file input, the focus shifts to the browser's file dialog. This triggers the input's blur event, which in turn runs the validation logic. This can lead to immediate validation errors being displayed even before the user has selected a file, which is not ideal. To fix this, please implement an internal boolean flag (e.g., _hasActivation).
Set this flag to true on the input's click event (activation), and reset it to false on the change or cancel events. Then, modify the blur handler to skip validation if _hasActivation is true.

Comment on lines 131 to 135
this.input.value = '';
this._formValue.value = this._formValue.defaultValue;

this.requestUpdate();
this._updateValidity();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.input.value = '';
this._formValue.value = this._formValue.defaultValue;
this.requestUpdate();
this._updateValidity();
this.input.value = '';
super._restoreDefaultValue();

Wouldn't that do the same thing?

Comment on lines 146 to 147
if (!this.input) return null;
return this.input.files;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but it's easier to parse at a glance

Suggested change
if (!this.input) return null;
return this.input.files;
return this.input?.files ?? null;

<input
id=${this.inputId}
part=${partNameMap(this.resolvePartNames('input'))}
name=${ifDefined(this.name)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to set a name attribute on the underlying input since it does not participate in form submission

Suggested change
name=${ifDefined(this.name)}

?multiple=${this.multiple}
tabindex=${this.tabIndex}
accept=${ifDefined(this.accept === '' ? undefined : this.accept)}
aria-invalid=${this.invalid ? 'true' : 'false'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aria-invalid=${this.invalid ? 'true' : 'false'}
aria-invalid=${this.invalid}

@simeonoff simeonoff requested a review from rkaraivanov April 1, 2025 12:13
@rkaraivanov rkaraivanov changed the title feat(file-input): add new component feat(file-input): Added file input component Apr 3, 2025
@rkaraivanov rkaraivanov merged commit e8991e7 into master Apr 3, 2025
5 checks passed
@rkaraivanov rkaraivanov deleted the simeonoff/input/file-type branch April 3, 2025 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]: Support type="file" for File Upload
3 participants